Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cloudflare): add queue and scheduled support WIP #1579

Closed
wants to merge 8 commits into from

Conversation

Hebilicious
Copy link
Member

@Hebilicious Hebilicious commented Aug 15, 2023

πŸ”— Linked issue

Related #1571

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@Hebilicious Hebilicious changed the title feat(cloudflare): add queue and scheduled support feat(cloudflare): add queue and scheduled support WIP Aug 15, 2023
env: CFModuleEnv,
context: ExecutionContext
) {
return nitroApp.hooks.callHook("cloudflare:queue", batch, env, context);
Copy link
Member

@pi0 pi0 Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure forward/backward compatibility would be nice to send them all as { batch, env, context }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try.
My main issue with this is that from what I can see, return nitroApp.hooks.callHook("test", () => 1) doesn't return 1 ?
Is there any other API suggestion here ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second arg is passed to listeners it is not a callback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, since we are supposed to return something, should we do something like this ?

    const hookResponse = { response: new Response() }
    await nitroApp.hooks.callHook("cloudflare-module:queue", {
      batch,
      env,
      context,
      hookResponse
    });
    return hookResponse.response

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah for handling, yes we might pass a callback like sendResponse (from the context object)

Copy link
Member Author

@Hebilicious Hebilicious Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export default defineNitroPlugin((app) => {
  app.hooks.hook("cloudflare:scheduled", (event) => {
    event.waitUntil(new Promise((resolve) => resolve("scheduled:cloudflare")));
  });

  app.hooks.hook(
    "cloudflare-module:scheduled",
    ({ controller, env, context }) => {
      context.waitUntil(
        new Promise((resolve) => resolve("scheduled:cloudflare-module"))
      );
    }
  );

  app.hooks.hook("cloudflare:queue", (event) => {
    event.sendResponse = () => "queued:cloudflare";
  });

  app.hooks.hook("cloudflare-module:queue", ({ batch, env, context }) => {
    context.sendResponse = () => "queued:cloudflare-module";
  });
});

import { withoutBase } from "ufo";
import { requestHasBody } from "../utils";
import { nitroApp } from "#internal/nitro/app";
import { useRuntimeConfig } from "#internal/nitro";
import { getPublicAssetMeta } from "#internal/nitro/virtual/public-assets";

addEventListener("scheduled", (event) => {
return nitroApp.hooks.callHook("cloudflare:scheduled", event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make sure if the format is same or similar to workers?

Copy link
Member Author

@Hebilicious Hebilicious Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They have different arguments, but typing doesn't work properly. Should we go with cloudflare-module:scheduled and cloudflare:scheduled instead?
image
the first event is typed as any

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the event type also different between them?

Copy link
Member Author

@Hebilicious Hebilicious Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the event type also different between them?

According to Typescript, they are the same for scheduled. But they aren't for queues.

Edit: Nevermind they are totally different

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least object syntax would make them more similar... This is a low-level hook anyway btw so i won't worry too much about differences. We can set an indicator key like isWorker/isModule if needed.

Copy link
Member Author

@Hebilicious Hebilicious Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interface CloudflareModuleRuntimeHooks {
  "cloudflare:scheduled": (moduleArgs: {
    controller: ScheduledController;
    env: CloudflareModuleEnv;
    context: ExecutionContext;
  }) => any;
  "cloudflare:queue": (moduleArgs: {
    batch: MessageBatch;
    env: CloudflareModuleEnv;
    context: ExecutionContext;
  }) => any;
}

interface CloudflareSWRuntimeHooks {
  "cloudflare:scheduled": (event: ScheduledEvent) => any;
  "cloudflare:queue": (event: QueueEvent) => any;
}

This is their signature at the moment, they don't have any common properties.

Note that addEventListener can also accept a 3rd optional argument that could be specified with a 2nd hook.

export interface EventTargetAddEventListenerOptions {
  capture?: boolean;
  passive?: boolean;
  once?: boolean;
  signal?: AbortSignal;
}

@trylovetom
Copy link
Contributor

Hello @pi0 @Hebilicious

I noticed that the PR is currently in a draft status. May I ask if there are any follow-up plans or an expected release timeline for this?

I believe it brings tremendous value and can truly elevate the cloudflare project to another level.

@pi0
Copy link
Member

pi0 commented Aug 29, 2023

@trylovetom Surely it is in the roadmap for next minor version of Nitro. I expect to release alongside new Tasks feature so that you can easily test everything universally before deploying!

@trylovetom
Copy link
Contributor

Surely it is in the roadmap for next minor version of Nitro. I expect to release alongside new Tasks feature so that you can easily test everything universally before deploying!

That's fantastic news! Is there an RFC or a trial version I can refer to or test out? Perhaps I can assist or provide some user feedback. I'm eager to see how it integrates to the project.

@pi0
Copy link
Member

pi0 commented Sep 8, 2023

Hi @Hebilicious and thanks again for this PR. More i think it makes more sense that we introduce this feature on top of (universal) nitro tasks (#1571 (comment)) instead of directly supporting provider-flavored APIs and hooks.

I am thinking of closing this draft to clarify our roadmap but i would happy to have your help to build this feature on top of nitro nitro tasks or revisit if that took longer than expected and you think it is critical to support this feature ahead of the time.

@pi0 pi0 closed this Sep 8, 2023
@Hebilicious
Copy link
Member Author

@pi0 My personal thoughts is that we need 2 features :

Should I open 2 issues to track these as a follow-up ?

I do think this is going to be a big chunk of work to make them platform agnostic and to integrate with all providers; while introducing platform specific hooks (maybe under an experimental flag?) is relatively fast and clean.

Regarding whether it's critical or not, personally, if I need this, I would use a custom preset.
I think documenting how to make a custom custom preset to use these 2 features, is cleaner and easier, as there's not a need to learn any extra syntax and to figure out how to do something that is already well explained in the cloudflare docs. But I understand that we shouldn't recommend them either ... though call... Maybe we could add an experimental flag for custom presets ? πŸ€”

@pi0
Copy link
Member

pi0 commented Sep 8, 2023

No please wait for me to open RFC/POC for Tasks. We can discuss Queues in the next steps. (could be on top as a module. Not sure if queue can be generic feature easily while tasks can)

I do think this is going to be a big chunk of work to make them platform agnostic and to integrate with all providers

This is all mission of Nitro to do heavy tasks like this. Don't worry we can make it happen!

Regarding whether it's critical or not, personally, if I need this, I would use a custom preset.
I think documenting how to make a custom custom preset to use these 2 features, is cleaner and easier, as there's not a need to learn any extra syntax and to figure out how to do something that is already well explained in the cloudflare docs. But I understand that we shouldn't recommend them either ... though call... Maybe we could add an experimental flag for custom presets

Surely as i mentioned above and in linked issue, the initiatives behind introducing hooks was to speed up introducing it for cloudflare, which took longer than expected with your PR and also tasks is one of my main priorities to deliver ASAP.

Again, i wouldn't recommend using custom presets, for same reasons we don't recommend making a custom Vue setup for Nuxt applications. The job of Nitro, Nuxt and other meta frameworks, is to keep users away from boilerplate and make sure they are using best optimized practices. I made both Nuxt and Nitro fully extendable so we don't "block" users of doing something like this just like you can do today but neither don't advice them to do as is a dangerous territory in regards of promised stability. I hope you understand this concept.

@Hebilicious
Copy link
Member Author

Hebilicious commented Sep 8, 2023

No please wait for me to open RFC/POC for Tasks. We can discuss Queues in the next steps. (could be on top as a module. Not sure if queue can be generic feature easily while tasks can)

I do think this is going to be a big chunk of work to make them platform agnostic and to integrate with all providers

This is all mission of Nitro to do heavy tasks like this. Don't worry we can make it happen!

Regarding whether it's critical or not, personally, if I need this, I would use a custom preset.
I think documenting how to make a custom custom preset to use these 2 features, is cleaner and easier, as there's not a need to learn any extra syntax and to figure out how to do something that is already well explained in the cloudflare docs. But I understand that we shouldn't recommend them either ... though call... Maybe we could add an experimental flag for custom presets

Surely as i mentioned above and in linked issue, the initiatives behind introducing hooks was to speed up introducing it for cloudflare, which took longer than expected with your PR and also tasks is one of my main priorities to deliver ASAP.

Again, i wouldn't recommend using custom presets, for same reasons we don't recommend making a custom Vue setup for Nuxt applications. The job of Nitro, Nuxt and other meta frameworks, is to keep users away from boilerplate and make sure they are using best optimized practices. I made both Nuxt and Nitro fully extendable so we don't "block" users of doing something like this just like you can do today but neither don't advice them to do as is a dangerous territory in regards of promised stability. I hope you understand this concept.

I understand this concept, I'm reiterating the custom preset point because this PR was closed.
In that regard, I am happy to move forward and re-open this PR (ie add docs), and get it merged if that implementation is satisfactory; or to drop it entirely if you don't think it's important.

@pi0 pi0 deleted the feat/cloudflare-queues-scheduled branch October 28, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants